-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix --nodelete behaviour on dev command #4662
base: main
Are you sure you want to change the base?
Conversation
11ac5b5
to
689aeac
Compare
Coverage report
Show files with reduced coverage 🔻
Test suite run success1917 tests passing in 871 suites. Report generated by 🧪jest coverage report action from e97fb51 |
This comment has been minimized.
This comment has been minimized.
689aeac
to
043c81d
Compare
This fixes an issue where the --nodelete flag was not being respected when running the dev command. This was due to the fact that the flag was not being passed to the theme-fs utility function.
043c81d
to
e97fb51
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/themes/types.d.ts@@ -28,6 +28,7 @@ export interface ThemeFileSystemOptions {
only?: string[];
};
notify?: string;
+ noDelete?: boolean;
}
/**
* Represents a theme on the file system.
|
@@ -178,7 +178,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti | |||
} | |||
|
|||
const handleFileDelete = (themeId: string, adminSession: AdminSession, fileKey: string) => { | |||
if (isFileIgnored(fileKey)) return | |||
if (isFileIgnored(fileKey) || options?.noDelete) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a test case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩 LGTM! We could probably use a test to solidify this in the contract, imo, but I'll leave that up to you and Guilherme
@@ -178,7 +178,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti | |||
} | |||
|
|||
const handleFileDelete = (themeId: string, adminSession: AdminSession, fileKey: string) => { | |||
if (isFileIgnored(fileKey)) return | |||
if (isFileIgnored(fileKey) || options?.noDelete) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test for this? Maybe for the ignore
as well ❤️
WHY are these changes introduced?
Fixes: https://github.com/Shopify/develop-advanced-edits/issues/370
While fixing up some of the documentation relating to using the
--nodelete
flag, I noticed that files were still being deleted remotely when usingtheme dev --nodelete
.WHAT is this pull request doing?
The
mountThemeFileSystem
was not receiving the--nodelete
flag if it was being used. I added it to theThemeFileSystemOptions
type and now we check if it present in thehandleFileDelete
method. If it is, we return early.How to test your changes?
Run the current build and use the command
theme dev --nodelete
Make a file.
Delete the file.
You should see it be deleted remotely.
Pull down this branch
Build this branch
Run
theme dev --nodelete
Make a file
Delete the file
You will see it keeps the remote file.
Measuring impact
How do we know this change was effective? Please choose one:
Checklist